-
Notifications
You must be signed in to change notification settings - Fork 157
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add xxd package and change google_nvme_id permission #2412
Conversation
- This package it required for the google_nvme_id script added in our 30gcp-udev-rules overlay - We can drop it once we get https://bugzilla.redhat.com/show_bug.cgi?id=2182865 done Signed-off-by: Renata Ravanelli <rravanel@redhat.com>
- We need to execute google_nvme_id script, change the file permission to allow it Signed-off-by: Renata Ravanelli <rravanel@redhat.com>
# It will include xxd for FCOS and vim-common for RHCOS | ||
# We can drop it once we get https://bugzilla.redhat.com/show_bug.cgi?id=2182865 done |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
# It will include xxd for FCOS and vim-common for RHCOS | |
# We can drop it once we get https://bugzilla.redhat.com/show_bug.cgi?id=2182865 done | |
# It will include xxd for FCOS and vim-common for RHCOS. | |
# We can drop it once we get https://bugzilla.redhat.com/show_bug.cgi?id=2182865 | |
# done because the package should name all dependencies. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM - one suggestion
Before going through the whole backport process across the two repos, it would be good to sanity-check at this point that the expected symlinks show up in an FCOS booted in GCP. If the concerned NVMe devices are the default ones, then we can add a non-exclusive test for it gated on |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't want us to keep pilling workaround for this. Can we investigate if we really need xxd
here of if we can use something else? We are generally very conservative when adding new user facing packages in FCOS and this change should follow the same process.
This change should land with a kola test that verifies it: coreos/fedora-coreos-tracker#1457
I think we need
After install vim-common and run, check we have
It would be better if do testing with FCOS that includes this patch. @ravanelli is there any guidance I can build gcp image and do testing? |
It's a dep of the script. Once https://bugzilla.redhat.com/show_bug.cgi?id=2182865 happens, it would've been pulled in via the new subpackage that holds |
We generally look at dependencies for packages that we add and consider whether we want them included as well. For example right now we have the According to https://manpages.ubuntu.com/manpages/xenial/man1/nvme-id-ns.1.html, this udev rule is parsing JSON output and that is likely better done with |
Directly using
update
to
Test works like this:
|
See https://manpages.ubuntu.com/manpages/xenial/man1/nvme-id-ns.1.html: you have to remove the |
Just to surface a discussion we had about this. We aren't going to ship it into 4.13 GA so we can more take our time with this. The suggestion is to go through the New Package Request flow for |
I think the problem here is that we need the binary output. Basically GCP is storing information in the "vendor data" and the script is dumping the raw data out to access it. Here are some examples:
so when we pass
so that's not really useful either. Indeed IMO the best course of action is to use
so we'd end up with:
I'm not sure on the |
actually
|
I guess the bash warning isn't new so we can ignore it:
|
`google_nvme_id` script currently uses `xxd` to parse nvme device info, but we need to install `xxd` package for fedora, `vim-common` and `vim-filesystem` for centos (or rhel) before using it. Replace it with `cut` and we do not need to install additional packages. See coreos/fedora-coreos-config#2412 (comment)
`google_nvme_id` script currently uses `xxd` to parse nvme device info, but we need to install additional package `xxd` for fedora, `vim-common` and `vim-filesystem` for centos (or rhel) before using it. Replace it with `cut` and we do not need to install additional packages. See coreos/fedora-coreos-config#2412 (comment)
`google_nvme_id` script currently uses `xxd` to parse nvme device info, but we need to install additional package `xxd` for fedora, `vim-common` and `vim-filesystem` for centos (or rhel) before using it. Replace it with `cut` and we do not need to install additional packages. See coreos/fedora-coreos-config#2412 (comment)
`google_nvme_id` script currently uses `xxd` to parse nvme device info, but we need to install additional package `xxd` for fedora, `vim-common` and `vim-filesystem` for centos (or rhel) before using it. Replace it with `cut` and we do not need to install additional packages. See coreos/fedora-coreos-config#2412 (comment)
`google_nvme_id` script currently uses `xxd` to parse nvme device info, but we need to install additional package `xxd` for fedora, `vim-common` and `vim-filesystem` for centos (or rhel) before using it. Replace it with `cut` and we do not need to install additional packages. See coreos/fedora-coreos-config#2412 (comment)
Thanks all for the suggestions, and good news is GoogleCloudPlatform/guest-configs#49 is merged. cc @ravanelli |
Thanks for investigation! |
Looks like @HuijingHei's PR has already been autoreleased: https://github.com/GoogleCloudPlatform/guest-configs/releases/tag/20230515.00. The Fedora package is hooked to release monitoring, so it should file an RHBZ soon for it. |
IMU, you mean https://bugzilla.redhat.com/show_bug.cgi?id=2204489 ? And @dustymabe 's PR |
I think we are good to close it now that we have all the fixes the need in upstream. |
Please note that GoogleCloudPlatform/guest-configs#49 had to be reverted. My analysis of the problem with GoogleCloudPlatform/guest-configs#49 which required that it be reverted can be found here. This might be helpful if you want to prepare another pull request for the guest-configs GCP repo. Cheers! |
`google_nvme_id` script currently uses `xxd` to parse nvme device info, but we need to install additional package `xxd` for fedora, `vim-common` and `vim-filesystem` for centos (or rhel) before using it. Replace it with `dd` and we do not need to install additional packages. See coreos/fedora-coreos-config#2412 (comment) We initially tried to replace it with cut, creating a different result expected. See: GoogleCloudPlatform#49 Discussing about the use of `dd`vs `cut`. Tests for Fedora CoreOS: nvme id-ns -b /dev/nvme0n1 | xxd -p --seek 384 | xxd -p -r | od -x 0000000 227b 6564 6976 6563 6e5f 6d61 2265 223a 0000020 6570 7372 7369 6574 746e 642d 7369 2d6b 0000040 2230 222c 6964 6b73 745f 7079 2265 223a 0000060 4550 5352 5349 4554 544e 7d22 0000 0000 0000100 0000 0000 0000 0000 0000 0000 0000 0000 * 0007200 nvme id-ns -b /dev/nvme0n1 | dd bs=1 skip=384 2>/dev/null | od -x 0000000 227b 6564 6976 6563 6e5f 6d61 2265 223a 0000020 6570 7372 7369 6574 746e 642d 7369 2d6b 0000040 2230 222c 6964 6b73 745f 7079 2265 223a 0000060 4550 5352 5349 4554 544e 7d22 0000 0000 0000100 0000 0000 0000 0000 0000 0000 0000 0000 * 0007200 Tests for Debian 12 nvme id-ns -b /dev/nvme0n1 | dd bs=1 skip=384 2>/dev/null | od -x 0000000 227b 6564 6976 6563 6e5f 6d61 2265 223a 0000020 6f6c 6163 2d6c 766e 656d 732d 6473 312d 0000040 2c22 6422 7369 5f6b 7974 6570 3a22 4c22 0000060 434f 4c41 535f 4453 7d22 0000 0000 0000 0000100 0000 0000 0000 0000 0000 0000 0000 0000 * 0007200 nvme id-ns -b /dev/nvme0n1 | dd bs=1 skip=384 2>/dev/null | od -x 0000000 227b 6564 6976 6563 6e5f 6d61 2265 223a 0000020 6f6c 6163 2d6c 766e 656d 732d 6473 312d 0000040 2c22 6422 7369 5f6b 7974 6570 3a22 4c22 0000060 434f 4c41 535f 4453 7d22 0000 0000 0000 0000100 0000 0000 0000 0000 0000 0000 0000 0000 * 0007200 Signed-off-by: Renata Ravanelli <rravanel@redhat.com>
`google_nvme_id` script currently uses `xxd` to parse nvme device info, but we need to install additional package `xxd` for fedora, `vim-common` and `vim-filesystem` for centos (or rhel) before using it. Replace it with `dd` and we do not need to install additional packages. See coreos/fedora-coreos-config#2412 (comment) We initially tried to replace it with cut, creating a different result than expected. See: GoogleCloudPlatform#49 Discussion about the use of `dd`vs `cut`. Tests for Fedora CoreOS: ``` nvme id-ns -b /dev/nvme0n1 | xxd -p --seek 384 | xxd -p -r | od -x 0000000 227b 6564 6976 6563 6e5f 6d61 2265 223a 0000020 6570 7372 7369 6574 746e 642d 7369 2d6b 0000040 2230 222c 6964 6b73 745f 7079 2265 223a 0000060 4550 5352 5349 4554 544e 7d22 0000 0000 0000100 0000 0000 0000 0000 0000 0000 0000 0000 * 0007200 nvme id-ns -b /dev/nvme0n1 | dd bs=1 skip=384 2>/dev/null | od -x 0000000 227b 6564 6976 6563 6e5f 6d61 2265 223a 0000020 6570 7372 7369 6574 746e 642d 7369 2d6b 0000040 2230 222c 6964 6b73 745f 7079 2265 223a 0000060 4550 5352 5349 4554 544e 7d22 0000 0000 0000100 0000 0000 0000 0000 0000 0000 0000 0000 * 0007200 ``` Tests for Debian 12 ``` nvme id-ns -b /dev/nvme0n1 | dd bs=1 skip=384 2>/dev/null | od -x 0000000 227b 6564 6976 6563 6e5f 6d61 2265 223a 0000020 6f6c 6163 2d6c 766e 656d 732d 6473 312d 0000040 2c22 6422 7369 5f6b 7974 6570 3a22 4c22 0000060 434f 4c41 535f 4453 7d22 0000 0000 0000 0000100 0000 0000 0000 0000 0000 0000 0000 0000 * 0007200 nvme id-ns -b /dev/nvme0n1 | dd bs=1 skip=384 2>/dev/null | od -x 0000000 227b 6564 6976 6563 6e5f 6d61 2265 223a 0000020 6f6c 6163 2d6c 766e 656d 732d 6473 312d 0000040 2c22 6422 7369 5f6b 7974 6570 3a22 4c22 0000060 434f 4c41 535f 4453 7d22 0000 0000 0000 0000100 0000 0000 0000 0000 0000 0000 0000 0000 * 0007200 ``` Signed-off-by: Renata Ravanelli <rravanel@redhat.com>
`google_nvme_id` script currently uses `xxd` to parse nvme device info, but we need to install additional package `xxd` for fedora, `vim-common` and `vim-filesystem` for centos (or rhel) before using it. Replace it with `dd` and we do not need to install additional packages. See coreos/fedora-coreos-config#2412 (comment) We initially tried to replace it with cut, creating a different result than expected. See: GoogleCloudPlatform#49 Discussion about the use of `dd`vs `cut`. Tests for Fedora CoreOS: ``` nvme id-ns -b /dev/nvme0n1 | xxd -p --seek 384 | xxd -p -r | od -x 0000000 227b 6564 6976 6563 6e5f 6d61 2265 223a 0000020 6570 7372 7369 6574 746e 642d 7369 2d6b 0000040 2230 222c 6964 6b73 745f 7079 2265 223a 0000060 4550 5352 5349 4554 544e 7d22 0000 0000 0000100 0000 0000 0000 0000 0000 0000 0000 0000 * 0007200 nvme id-ns -b /dev/nvme0n1 | dd bs=1 skip=384 2>/dev/null | od -x 0000000 227b 6564 6976 6563 6e5f 6d61 2265 223a 0000020 6570 7372 7369 6574 746e 642d 7369 2d6b 0000040 2230 222c 6964 6b73 745f 7079 2265 223a 0000060 4550 5352 5349 4554 544e 7d22 0000 0000 0000100 0000 0000 0000 0000 0000 0000 0000 0000 * 0007200 ``` Tests for Debian 12 ``` nvme id-ns -b /dev/nvme0n1 | dd bs=1 skip=384 2>/dev/null | od -x 0000000 227b 6564 6976 6563 6e5f 6d61 2265 223a 0000020 6f6c 6163 2d6c 766e 656d 732d 6473 312d 0000040 2c22 6422 7369 5f6b 7974 6570 3a22 4c22 0000060 434f 4c41 535f 4453 7d22 0000 0000 0000 0000100 0000 0000 0000 0000 0000 0000 0000 0000 * 0007200 nvme id-ns -b /dev/nvme0n1 | dd bs=1 skip=384 2>/dev/null | od -x 0000000 227b 6564 6976 6563 6e5f 6d61 2265 223a 0000020 6f6c 6163 2d6c 766e 656d 732d 6473 312d 0000040 2c22 6422 7369 5f6b 7974 6570 3a22 4c22 0000060 434f 4c41 535f 4453 7d22 0000 0000 0000 0000100 0000 0000 0000 0000 0000 0000 0000 0000 * 0007200 ``` Signed-off-by: Renata Ravanelli <rravanel@redhat.com>
* Replace xxd with dd for google_nvme_id `google_nvme_id` script currently uses `xxd` to parse nvme device info, but we need to install additional package `xxd` for fedora, `vim-common` and `vim-filesystem` for centos (or rhel) before using it. Replace it with `dd` and we do not need to install additional packages. See coreos/fedora-coreos-config#2412 (comment) We initially tried to replace it with cut, creating a different result than expected. See: #49 Discussion about the use of `dd`vs `cut`. Tests for Fedora CoreOS: ``` nvme id-ns -b /dev/nvme0n1 | xxd -p --seek 384 | xxd -p -r | od -x 0000000 227b 6564 6976 6563 6e5f 6d61 2265 223a 0000020 6570 7372 7369 6574 746e 642d 7369 2d6b 0000040 2230 222c 6964 6b73 745f 7079 2265 223a 0000060 4550 5352 5349 4554 544e 7d22 0000 0000 0000100 0000 0000 0000 0000 0000 0000 0000 0000 * 0007200 nvme id-ns -b /dev/nvme0n1 | dd bs=1 skip=384 2>/dev/null | od -x 0000000 227b 6564 6976 6563 6e5f 6d61 2265 223a 0000020 6570 7372 7369 6574 746e 642d 7369 2d6b 0000040 2230 222c 6964 6b73 745f 7079 2265 223a 0000060 4550 5352 5349 4554 544e 7d22 0000 0000 0000100 0000 0000 0000 0000 0000 0000 0000 0000 * 0007200 ``` Tests for Debian 12 ``` nvme id-ns -b /dev/nvme0n1 | dd bs=1 skip=384 2>/dev/null | od -x 0000000 227b 6564 6976 6563 6e5f 6d61 2265 223a 0000020 6f6c 6163 2d6c 766e 656d 732d 6473 312d 0000040 2c22 6422 7369 5f6b 7974 6570 3a22 4c22 0000060 434f 4c41 535f 4453 7d22 0000 0000 0000 0000100 0000 0000 0000 0000 0000 0000 0000 0000 * 0007200 nvme id-ns -b /dev/nvme0n1 | dd bs=1 skip=384 2>/dev/null | od -x 0000000 227b 6564 6976 6563 6e5f 6d61 2265 223a 0000020 6f6c 6163 2d6c 766e 656d 732d 6473 312d 0000040 2c22 6422 7369 5f6b 7974 6570 3a22 4c22 0000060 434f 4c41 535f 4453 7d22 0000 0000 0000 0000100 0000 0000 0000 0000 0000 0000 0000 0000 * 0007200 ``` Signed-off-by: Renata Ravanelli <rravanel@redhat.com> * Revert "debian packaging: add xxd dependency (#55)" This reverts commit 480f02a.
See: https://issues.redhat.com/browse/OCPBUGS-7582